Deep strict evaluation of Response #36
Conversation
|
Also, if the response is large, the whole thing needs to sit in memory before it can be sent, which would be pretty bad for video streaming sites. Cale on IRC mentions that if Happstack WERE to A better solution might be to trap errors that are thrown from within It's very hard to say what the right thing is. Suppose you're streaming a video, and there's a bottom value halfway through it. I'll probably close this PR and create an issue instead. |
|
Forcing everything into RAM is definitely a no-go. In fact, we had work to make sure that did not happen by accident. Forgetting about the specifics of the implementation, I think there is a "can't have your cake and eat it too" problem here. One the one hand we would like to be able to generate the response body on the fly. Currently we do that via a lazy string -- but it could be done via conduits, pipes, machines, etc. But, since it is happening on the fly, there is always a chance for something to go wrong in the middle of generating the response body. On the other hand, you seem to want to return a '500 internal server error' response. But, that would require us to go back in time. When sending a response, we first send the response headers with the response code (such as 200 OK), and then we send the response body. So, if an error occurs while generating the response body, it's too late to send 500, because the response code has already been sent. So, I guess the question is -- what is the correct way to handle this issue in the HTTP system. Does the spec provide some way to say 'whoops, I said this was ok, but now I realize there is a problem'. I think maybe when using chunked responses, there is some mechanism for that. But it has been a long time since I read the spec. With the way things are currently implement (lazy by default), I believe that you can, in your application, force the response bodies to be strict. So that gives you the most flexibility. In fact, if you look at the file serving stuff we already provide the option to serve files using strict means rather than lazy, You could also create a filter that would force response bodies to be strict automatically (though you'd need some sanity check to ensure you don't force a whole video into RAM). |
Consider this server: it attempts to return a string with status 200,
however the string's value is `undefined`.
import Control.Monad.Catch (SomeException, handle)
main :: IO ()
main = do
let sendResponse = ok $ toResponse (fromJust Nothing :: String)
simpleHTTP nullConf (handle errorPage sendResponse)
where
errorPage :: SomeException -> ServerPart Response
errorPage _ = (internalServerError $ toResponse "Custom error page!")
On each request, this server replies with "200 OK" with no
content (even though philosophically there was a clear internal server
error, and furthermore there is an error handler present). At least
it outputs "HTTP request failed with: Maybe.fromJust: Nothing" to
stderr.
This patch makes it possible to deeply strictly evaluate Response
objects before sending the HTTP response. Then errors like these can
be caught and an error page shown. The application can be changed to
something like this:
import Control.DeepSeq (deepseq)
import Control.Monad.Catch (SomeException, handle)
handleServerPartError :: ServerPart Response -> ServerPart Response
handleServerPartError s = handle errorPage $ do
res <- s
deepseq res (return res)
where
errorPage :: SomeException -> ServerPart Response
errorPage _ = (internalServerError $ toResponse "Custom error page!")
main :: IO ()
main = do
let sendResponse = ok $ toResponse (fromJust Nothing :: String)
simpleHTTP nullConf (handleServerPartError sendResponse)
Which deeply, strictly evaluates the Response, revealing the
`undefined` that is present and displaying the custom error page.
Not all requests (streaming videos, for example) can or should be
deeply strictly evaluated before being sent to the client. However,
for requests like those, the client often knows not to fully trust the
200 status code of the response, and likely has some error handling
built in if the response data does not stream in in the expected
format. For most HTTP requests, the status code can be trusted, and
client applications should not need to have any special handling.
negatratoron
force-pushed
the
strict-response
branch
from
e23b4b2
to
bd7ad41
Compare
|
I just changed this patch so that it doesn't internally evaluate all Response objects, but still enables them to be deeply evaluated if the application writer wishes to. I changed the commit message too, to match what is actually done. |
|
I'll say that at work these not being strictly evaluated led to us missing application errors once upon a time. If I recall we basically just I mostly mention it to add a small weight to this issue and to ask about any other downsides that approach may have. |
Without this change, Happstack's
failResponseis not always sent.For instance:
This server tries to send a string, but can't because the string's
value is bottom. Before this change, the server replies with status
200 and empty content, while outputting "HTTP request failed with:
Maybe.fromJust: Nothing" to stderr. This is because the response is
only evaluated into WHNF inside the exception handler in Handler.hs,
and the exception is caught by the surrounding catch block in
Listen.hs. After this change, the server replies with status 500 and
the
failResponsepage.It's possible that this slows down the Happstack server. Some parts
of the Response object may be evaluated but not needed. Strictly
evaluating the response at a finer level might be necessary.
Additionally, with NFData Response, clients can call
deepseqthemselves on the response object in order to catch any bottom values
that it contains, log them and display a custom error page:
Let me know if you want to merge in this patch. I'll be using it in
my own applications for sure, unless it proves to cause any
problems. :)